Reorganize tctl commands to not require an auth client by default by vapopov · Pull Request #48894 · gravitational/teleport

vapopov · 2024-11-13T14:37:18Z

In this PR added ability to declare tctl commands which not required auth client such as version command

zmb3 · 2024-11-13T15:06:25Z

I don't love this approach but I'm struggling to come up with a better idea at the moment.. 🤔

vapopov · 2024-11-13T16:08:49Z

I don't love this approach but I'm struggling to come up with a better idea at the moment.. 🤔

@zmb3 I tried couple other options but they also look not perfect, like to make auth client initialization as lazy loading so only commands which requires auth going to init connection, and another one - not return error if auth client failed to init, so in commands client might be nil

TryRun(ctx context.Context, cmd string, client *authclient.Client)

rosstimothy

I agree with Zac, I'm not particularly fond of splitting up the commands. We could potentially expand on your suggestion to have the client be lazily constructed and instead of

TryRun(ctx context.Context, cmd string, client *authclient.Client)

we could do

TryRun(ctx context.Context, cmd string, clientFunc func() *authclient.Client)

In that scenario, the logic of building the client can stay where it is, but only be executed if the command requires an authenticated connection to the cluster.

…tctl-commands

vapopov · 2024-11-15T20:44:25Z

I've refactored to have separate auth client initialization after command matching
commands that do not require an authentication client: tctl version|top|fido2|touchid|webauthnwin

zmb3

I'm liking this approach better. 👍

zmb3 · 2024-11-20T21:08:07Z

 	switch cmd {
 	case a.authGenerate.FullCommand():
-		err = a.GenerateKeys(ctx, client)
+		client, clientClose, err := clientFunc(ctx)


There's a lot of repeated code here. Would it be simpler to run the clientFunc prior to the switch?

In this case we init auth client (start checking profile folder, establish connection, request ping) before actual matching the command has to be run. We need to identify what command it is by FullCommand() and then each command decides if auth client is required

Replace logrus to use slog

…tctl-commands

vapopov · 2024-12-02T17:07:31Z

would appreciate of review of this PR

Joerger

LGTM, approach seems reasonable. Just left a suggestion for the naming.

…tctl-commands

vapopov · 2024-12-09T16:24:08Z

one more review by any chance

hugoShaka

LGTM once comments are resolved

hugoShaka · 2024-12-11T19:21:31Z

+func (c *fido2Command) TryRun(ctx context.Context, cmd string, _ commonclient.InitFunc) (match bool, err error) {
+	return c.impl.TryRun(ctx, cmd)


This PR makes the fido2 command work even without a valid client, nice :)

…lient Code review changes

…tctl-commands

…8894) * Reorganize tctl commands to have commands not required auth client * Replace auth client with lazy loading approach * Fix linter warning * Replace camel case in import alias Replace logrus to use slog * Rename close function * Refactor plugin commands to use interface of auth client and plugin client Code review changes * Refactor workload identity commands * Add access to global config for the commands * Add test checking all tctl commands match process * Fix golangci-lint warnings

…8894) (#50966) * Reorganize tctl commands to have commands not required auth client * Replace auth client with lazy loading approach * Fix linter warning * Replace camel case in import alias Replace logrus to use slog * Rename close function * Refactor plugin commands to use interface of auth client and plugin client Code review changes * Refactor workload identity commands * Add access to global config for the commands * Add test checking all tctl commands match process * Fix golangci-lint warnings

…8894) (#50968) * Reorganize tctl commands to have commands not required auth client * Replace auth client with lazy loading approach * Fix linter warning * Replace camel case in import alias Replace logrus to use slog * Rename close function * Refactor plugin commands to use interface of auth client and plugin client Code review changes * Refactor workload identity commands * Add access to global config for the commands * Add test checking all tctl commands match process * Fix golangci-lint warnings

…avitational#48894) * Reorganize tctl commands to have commands not required auth client * Replace auth client with lazy loading approach * Fix linter warning * Replace camel case in import alias Replace logrus to use slog * Rename close function * Refactor plugin commands to use interface of auth client and plugin client Code review changes * Refactor workload identity commands * Add access to global config for the commands * Add test checking all tctl commands match process * Fix golangci-lint warnings

vapopov requested review from hugoShaka and rosstimothy November 13, 2024 14:37

github-actions Bot added the size/sm label Nov 13, 2024

github-actions Bot requested a review from Joerger November 13, 2024 14:37

github-actions Bot added tctl tctl - Teleport admin tool tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels Nov 13, 2024

github-actions Bot requested a review from r0mant November 13, 2024 14:37

vapopov force-pushed the vapopov/reorgonize-tctl-commands branch from ceef55e to 3d57ff8 Compare November 13, 2024 14:37

Reorganize tctl commands to have commands not required auth client

fa9f372

vapopov force-pushed the vapopov/reorgonize-tctl-commands branch from 3d57ff8 to fa9f372 Compare November 13, 2024 14:39

rosstimothy reviewed Nov 14, 2024

View reviewed changes

Comment thread tool/tctl/common/tctl.go Outdated

Comment thread tool/tctl/common/tctl.go Outdated

Comment thread tool/tctl/common/cmds.go Outdated

vapopov added 2 commits November 15, 2024 14:55

Replace auth client with lazy loading approach

af7baa1

Merge remote-tracking branch 'origin/master' into vapopov/reorgonize-…

7b32cfc

…tctl-commands

vapopov added the no-changelog Indicates that a PR does not require a changelog entry label Nov 15, 2024

Fix linter warning

e7d0081

vapopov requested review from rosstimothy and zmb3 November 18, 2024 21:50

zmb3 reviewed Nov 20, 2024

View reviewed changes

vapopov added 2 commits November 21, 2024 12:38

Replace camel case in import alias

0ea5a68

Replace logrus to use slog

Merge remote-tracking branch 'origin/master' into vapopov/reorgonize-…

b6fe948

…tctl-commands

vapopov force-pushed the vapopov/reorgonize-tctl-commands branch from 7240ab5 to b6fe948 Compare November 21, 2024 19:02

vapopov requested a review from zmb3 November 25, 2024 19:44

Joerger approved these changes Dec 2, 2024

View reviewed changes

Comment thread tool/tctl/common/access_request_command.go Outdated

Merge remote-tracking branch 'origin/master' into vapopov/reorgonize-…

c68f24f

…tctl-commands

vapopov force-pushed the vapopov/reorgonize-tctl-commands branch from 0569781 to c68f24f Compare December 6, 2024 01:34

Rename close function

352fd16

Joerger approved these changes Dec 9, 2024

View reviewed changes

vapopov requested a review from espadolini December 11, 2024 16:56

hugoShaka approved these changes Dec 11, 2024

View reviewed changes

public-teleport-github-review-bot Bot removed request for espadolini and r0mant December 11, 2024 20:18

vapopov added 5 commits December 16, 2024 22:04

Refactor plugin commands to use interface of auth client and plugin c…

1b50d46

…lient Code review changes

Merge remote-tracking branch 'origin/master' into vapopov/reorgonize-…

8d8f135

…tctl-commands

Refactor workload identity commands

ecbbbf5

Add access to global config for the commands

8047189

Add test checking all tctl commands match process

4b1e596

vapopov force-pushed the vapopov/reorgonize-tctl-commands branch from 70123e6 to 4b1e596 Compare December 17, 2024 19:52

Fix golangci-lint warnings

cebf1b0

vapopov changed the title ~~Reorganize tctl commands to have commands not required auth client~~ Reorganize tctl commands to not require an auth client by default Dec 17, 2024

vapopov added this pull request to the merge queue Dec 17, 2024

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Dec 17, 2024

vapopov added this pull request to the merge queue Dec 17, 2024

Merged via the queue into master with commit 45f29a8 Dec 17, 2024

vapopov deleted the vapopov/reorgonize-tctl-commands branch December 17, 2024 21:13

vapopov mentioned this pull request Jan 10, 2025

[v17] Reorganize tctl commands to not require an auth client by default #50966

Merged

vapopov mentioned this pull request Jan 11, 2025

[v16] Reorganize tctl commands to not require an auth client by default #50968

Merged

vapopov mentioned this pull request Jul 24, 2025

Local-only tctl commands should not attempt to connect to Proxy/Auth #41560

Closed

		func (c *fido2Command) TryRun(ctx context.Context, cmd string, _ commonclient.InitFunc) (match bool, err error) {
		return c.impl.TryRun(ctx, cmd)

Conversation

vapopov commented Nov 13, 2024

Uh oh!

zmb3 commented Nov 13, 2024

Uh oh!

vapopov commented Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rosstimothy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vapopov commented Nov 15, 2024

Uh oh!

zmb3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zmb3 Nov 20, 2024

Choose a reason for hiding this comment

Uh oh!

vapopov Nov 20, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vapopov commented Dec 2, 2024

Uh oh!

Joerger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vapopov commented Dec 9, 2024

Uh oh!

hugoShaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hugoShaka Dec 11, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vapopov commented Nov 13, 2024 •

edited

Loading